Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minimize bundled translations #1775

Merged
merged 1 commit into from
Mar 29, 2021
Merged

Conversation

jotoeri
Copy link
Contributor

@jotoeri jotoeri commented Mar 22, 2021

Why?

On Password-Policy the usage of SettingsSection instead of a simple div blew up the package-size from 100 KiB to 330 KiB of which 140 KB are the l10n.js of vue-Components. So i thought about how to reduce this.

Idea

The current implementation inserts the pot-file as json, and packs this within the bundle. However, there is a huge overhead on that kind of storing. So the idea was to reduce the overhead by storing only necessary data.
As model i initially used the json-format as translations are stored within apps.

Approach

  • Unfortunately i didn't find a npm-package/a way to store in the desired format, the recommended package (po2json) seems to be unmaintained and breaks on plural-translations.
  • Therefore this approach now manually transforms/compresses the format and decompresses it before inserting into gettextBuilder.
  • I must admit on the one hand this feels a bit hacky, as it compresses to a custom format on webpack-build, just to store the data and decompresses then on l10n insertion manually.
    => Webpack-build compresses -> Component stores compressed data -> l10n.js decompresses when loading gettext
  • The meta-information out of the header currently gets lost, but as far as i see, this works without. To be sure, on could think about restoring the plural-forms, language and content-type headers.

Result

This approach reduces the size of the bundled l10n.js from 142 KB to 32 KB!
Thus, the bundled password_policy settings reduce from 346 KiB to 237 KiB, while e.g. the forms-app bundle reduces from 2.75 MiB to 2.21 MiB.

More Information:

The way translations are stored currently (Only 1 plural and 2 singular translations):

{
  "locale": "de_DE",
  "json": {
    "charset": "utf-8",
    "headers": {
      "Last-Translator": "Mario Siegmann <[email protected]>, 2020",
      "Language-Team": "German (Germany) (https://www.transifex.com/nextcloud/teams/64236/de_DE/)",
      "Content-Type": "text/plain; charset=UTF-8",
      "Language": "de_DE",
      "Plural-Forms": "nplurals=2; plural=(n != 1);"
    },
    "translations": {
      "": {
        "": {
          "msgid": "",
          "comments": {
            "translator": "\nTranslators:\nPhilipp Fischbeck <[email protected]>, 2020\nProfDrJones <[email protected]>, 2020\nMark Ziegler <[email protected]>, 2020\nMario Siegmann <[email protected]>, 2020\n"
          },
          "msgstr": [
            "Last-Translator: Mario Siegmann <[email protected]>, 2020\nLanguage-Team: German (Germany) (https://www.transifex.com/nextcloud/teams/64236/de_DE/)\nContent-Type: text/plain; charset=UTF-8\nLanguage: de_DE\nPlural-Forms: nplurals=2; plural=(n != 1);\n"
          ]
        },
        "%n Action": {
          "msgid": "%n Action",
          "msgid_plural": "%n Actions",
          "msgstr": [
            "%n Aktion",
            "%n Aktionen"
          ]
        },
        "Actions": {
          "msgid": "Actions",
          "comments": {
            "reference": "src/components/Actions/Actions.vue:254"
          },
          "msgstr": [
            "Aktionen"
          ]
        },
        "Activities": {
          "msgid": "Activities",
          "comments": {
            "reference": "src/components/EmojiPicker/EmojiPicker.vue:176"
          },
          "msgstr": [
            "Aktivitäten"
          ]
        },
      }
    }
  }
}

The way i would store it here (same translation Data!):

{
  "locale": "de_DE",
  "translations": {
    "%n Action": {                       // Plural translation needs some specific handling
      "pluralId": "%n Actions",
      "msgstr": [
        "%n Aktion",
        "%n Aktionen"
      ]
    },
    "Actions": "Aktionen",           // Singular Translation
    "Activities": "Aktivitäten",
  }
},

Colorful image of the bundles before:
grafik

Colorful image of the bundles after:
grafik

@jotoeri jotoeri added 3. to review Waiting for reviews discussion Need advices, opinions or ideas on this topic technical debt labels Mar 22, 2021
@raimund-schluessler
Copy link
Contributor

This approach reduces the size of the bundled l10n.js from 142 KB to 32 KB!
Thus, the bundled password_policy settings reduce from 346 KiB to 237 KiB, while e.g. the forms-app bundle forms reduces from 2.75 MiB to 2.21 MiB.

Why does the forms-app bundle size reduce more than the l10n.js file size? Is the same l10n.js file packed into the bundle multiple times?

@jotoeri
Copy link
Contributor Author

jotoeri commented Mar 22, 2021

Why does the forms-app bundle size reduce more than the l10n.js file size? Is the same l10n.js file packed into the bundle multiple times?

Yep, we currently import the components as single components (instead of nc/vue as a whole). And each component contains the l10n-file. You can see this also in the two bundle-analyzer-images.

@raimund-schluessler
Copy link
Contributor

Why does the forms-app bundle size reduce more than the l10n.js file size? Is the same l10n.js file packed into the bundle multiple times?

Yep, we currently import the components as single components (instead of nc/vue as a whole). And each component contains the l10n-file. You can see this also in the two bundle-analyzer-images.

Hm, I thought (or maybe more like hoped) that this would be optimized by webpack.

@skjnldsv
Copy link
Contributor

Hm, I thought (or maybe more like hoped) that this would be optimized by webpack.

I guess we have some improvements to do in our webpack config 🤔

webpack.common.js Outdated Show resolved Hide resolved
Copy link
Contributor

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Mar 22, 2021
@jotoeri jotoeri force-pushed the enh/minimize_translations branch 2 times, most recently from 12984fc to 0f82fe5 Compare March 22, 2021 13:07
@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 22, 2021
@raimund-schluessler
Copy link
Contributor

raimund-schluessler commented Mar 22, 2021

Hm, I thought (or maybe more like hoped) that this would be optimized by webpack.

I guess we have some improvements to do in our webpack config 🤔

By the way, the font files also get duplicated in the final bundle. I had a look around, but couldn't find any solution for deduplicating this. If anyone has an idea, it would be great, since duplicating l10n.js and the fonts really has a huge impact on the bundle size.

@nextcloud/javascript

@skjnldsv
Copy link
Contributor

Moving to webbpack5 might be a great thing to do and have better tree-shaking.

@raimund-schluessler
Copy link
Contributor

Moving to webbpack5 might be a great thing to do and have better tree-shaking.

Not sure. In one of my apps I have webpack 5.27.1 and the code is still present multiple times in the final bundle. But I will check if using webpack5 in the vue-components as well has any influence.

@jotoeri
Copy link
Contributor Author

jotoeri commented Mar 23, 2021

As far as I see, webpack sees the components from the outside as one single block. So i think having separate chunks for l10n and fonts should enable webpack to see it as separate requirement and therefore enable to import it only once... I also tried that, but didn't manage to import it on a quick-shot. Seems to need some specific handling on the apps (or components?) to import the chunk then.

EDIT: Btw. also nice would be if we manage to import the chunk conditionally then. So as password_policy does not use the info-icon of settingsSection, the iconfonts package could be left out completely.... 🤔

@jotoeri
Copy link
Contributor Author

jotoeri commented Mar 23, 2021

⬆️ Small fixup to optimize plural storing a bit.

@jotoeri jotoeri mentioned this pull request Mar 23, 2021
3 tasks
@jotoeri
Copy link
Contributor Author

jotoeri commented Mar 23, 2021

Opened a separate issue for deduplication, so we can keep track there. #1779

@jotoeri jotoeri removed the discussion Need advices, opinions or ideas on this topic label Mar 23, 2021
Signed-off-by: Jonas Rittershofer <[email protected]>
@jotoeri
Copy link
Contributor Author

jotoeri commented Mar 28, 2021

Somebody else up for a second review here? 😊

@ChristophWurst ChristophWurst merged commit 6f27e09 into master Mar 29, 2021
@ChristophWurst ChristophWurst deleted the enh/minimize_translations branch March 29, 2021 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants